feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories #1369

gregmagolan · 2019-11-21T07:18:52Z

BREAKING CHANGES:

bazel run @nodejs//:npm is replaced with bazel run @nodejs//:npm_node_repositories and bazel run @nodejs//:yarn is replaced with bazel run @nodejs//:yarn_node_repositories. @nodejs//:yarn and @nodejs//:npm now run yarn & npm in the current working directory instead of on all of the package.json files in node_repositories().

@nodejs//:bin/node & @nodejs//:bin/node.cmd (on Windows) are no longer valid targets. Use @nodejs//:node instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/node.

@nodejs//:bin/yarn & @nodejs//:bin/yarn.cmd (on Windows) are no longer valid targets. Use @nodejs//:yarn instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/yarn.

@nodejs//:bin/npm & @nodejs//:bin/npm.cmd (on Windows) are no longer valid targets. Use @nodejs//:npm instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/npm.

While working on angular/angular#33927 and cleaning up node_repositories.bzl and in particular the aliases in the @nodejs repository, I came across this change which makes the API surface for the @nodejs repository much cleaner and more intuitive.

Users can just call @nodejs//:yarn on all platforms instead of @nodejs//:bin/yarn on osx/linux and @nodejs//:bin/yarn.cmd on Windows. Same difference for npm as well.

The targets @nodejs//:yarn_node_repositories and @nodejs//:npm_node_repositories are also better names for those entry points as they run yarn & npm on all of the package.json files in the node_repositories() repository rule.

Internally, the aliases match up with the actual entry point names so this makes the code much cleaner:

alias(name = "node", actual = "bin/node")
alias(name = "npm", actual = "bin/npm")
alias(name = "yarn", actual = "bin/yarn")
alias(name = "npm_node_repositories", actual = "bin/npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "bin/yarn_node_repositories")

and there is no longer a need to alias the bin/npm and bin/npm.cmd versions of the entry points in @nodejs//:BUILD.bazel as aliases to the aliases is sufficient:

alias(name = "node", actual = "@nodejs_darwin_amd64//:node")
alias(name = "npm", actual = "@nodejs_darwin_amd64//:npm")
alias(name = "yarn", actual = "@nodejs_darwin_amd64//:yarn")
alias(name = "npm_node_repositories", actual = "@nodejs_darwin_amd64//:npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "@nodejs_darwin_amd64//:yarn_node_repositories")

Also,

create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets
create @nodejs//:node_files, @nodejs//:npm_files & @nodejs//:yarn_files filegroups
cleanup & simplify node_repositories.bzl code
cleanup & simplify @nodejs//:BUILD.bazel file

@nodejs_darwin_amd64//:BUILD.bazel now looks like this:

# Generated by node_repositories.bzl
package(default_visibility = ["//visibility:public"])
exports_files([
  "run_npm.sh.template",
  "bin/node_repo_args.sh",
  "bin/nodejs/bin/node",
  "bin/nodejs/bin/npm",
  "bin/nodejs/bin/npx",
  "bin/yarnpkg/bin/yarn",
  "bin/node",
  "bin/npm",
  "bin/yarn",
  "bin/npm_node_repositories",
  "bin/yarn_node_repositories",
  ])
alias(name = "node_bin", actual = "bin/nodejs/bin/node")
alias(name = "npm_bin", actual = "bin/nodejs/bin/npm")
alias(name = "npx_bin", actual = "bin/nodejs/bin/npx")
alias(name = "yarn_bin", actual = "bin/yarnpkg/bin/yarn")
alias(name = "node", actual = "bin/node")
alias(name = "npm", actual = "bin/npm")
alias(name = "yarn", actual = "bin/yarn")
alias(name = "npm_node_repositories", actual = "bin/npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "bin/yarn_node_repositories")
filegroup(
  name = "node_files",
  srcs = [":node", ":node_bin"],
)
filegroup(
  name = "yarn_files",
  srcs = glob(["bin/yarnpkg/**"]) + [":node_files"],
)
filegroup(
  name = "npm_files",
  srcs = glob(["bin/nodejs/**"]) + [":node_files"],
)

and

@nodejs//:BUILD.bazel on OSX now looks like this:

# Generated by node_repositories.bzl
package(default_visibility = ["//visibility:public"])
# aliases for exports_files
alias(name = "run_npm.sh.template", actual = "@nodejs_darwin_amd64//:run_npm.sh.template")
alias(name = "bin/node_repo_args.sh", actual = "@nodejs_darwin_amd64//:bin/node_repo_args.sh")
# aliases for other aliases
alias(name = "node_bin", actual = "@nodejs_darwin_amd64//:node_bin")
alias(name = "npm_bin", actual = "@nodejs_darwin_amd64//:npm_bin")
alias(name = "npx_bin", actual = "@nodejs_darwin_amd64//:npx_bin")
alias(name = "yarn_bin", actual = "@nodejs_darwin_amd64//:yarn_bin")
alias(name = "node", actual = "@nodejs_darwin_amd64//:node")
alias(name = "npm", actual = "@nodejs_darwin_amd64//:npm")
alias(name = "yarn", actual = "@nodejs_darwin_amd64//:yarn")
alias(name = "npm_node_repositories", actual = "@nodejs_darwin_amd64//:npm_node_repositories")
alias(name = "yarn_node_repositories", actual = "@nodejs_darwin_amd64//:yarn_node_repositories")
alias(name = "node_files", actual = "@nodejs_darwin_amd64//:node_files")
alias(name = "yarn_files", actual = "@nodejs_darwin_amd64//:yarn_files")
alias(name = "npm_files", actual = "@nodejs_darwin_amd64//:npm_files")

Pre-req for angular/angular#33927 so we can call npm & yarn from within an npm_integration_test like so:

angular_integration_test(
    name = "cli_hello_world_test",
    commands = [
        # "$(location @nodejs//:npm_bin) install",
        # "$(location @nodejs//:npm_bin) run test",
        "$(location @nodejs//:yarn_bin) install",
        "$(location @nodejs//:yarn_bin) test",
    ],
    data = [
        # "@nodejs//:npm_bin",
        # "@nodejs//:npm_files",
        "@nodejs//:yarn_bin",
        "@nodejs//:yarn_files",
    ],
    npm_packages = {
        "//packages/animations:npm_package": "@angular/animations",
        "//packages/common:npm_package": "@angular/common",
        "//packages/compiler:npm_package": "@angular/compiler",
        "//packages/compiler-cli:npm_package": "@angular/compiler-cli",
        "//packages/core:npm_package": "@angular/core",
        "//packages/forms:npm_package": "@angular/forms",
        "//packages/language-service:npm_package": "@angular/language-service",
        "//packages/platform-browser:npm_package": "@angular/platform-browser",
        "//packages/platform-browser-dynamic:npm_package": "@angular/platform-browser-dynamic",
        "//packages/router:npm_package": "@angular/router",
        "//packages/zone.js:npm_package": "zone.js",
        "@npm//typescript": "typescript",
        "@npm//rxjs": "rxjs",
        "@npm//tslib": "tslib",
        "@npm//protractor": "protractor",
        "@npm//@angular/cli": "@angular/cli",
        "@npm//@types/node": "@types/node",
    },
    test_files = ":cli_hello_world_files",
)

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

alexeagle · 2019-11-21T16:58:45Z

Generally looks good to me, what are steps before you want review?

"The targets @nodejs//:yarn_node_repositories and @nodejs//:npm_node_repositories are also better names for those entry points as they run yarn & npm on all of the package.json files in the node_repositories() repository rule" - makes me think maybe the word "all" should appear in that rule name?

gregmagolan · 2019-11-21T19:47:12Z

Its ready for review now. Forgot that it was still in draft.

@nodejs//:yarn_all_node_repositories and @nodejs//:npm_all_node_repositories may be a little verbose. I wonder how many users are even using this feature now that yarn_install() and npm_install() symlink to the user's workspace?

Is the use case of,

node_repositories(["package.json", "foo/package.json", "bar/package.json"])

something that is useful for users?

Perhaps the recommended approach should be:

node_repositories()
yarn_install(name="npm", package_json="package.json")
yarn_install(name="npm2", package_json="foo/package.json")
yarn_install(name="npm3", package_json="bar/package.json")

or a user can also just run yarn or npm multiple times to replace the behavior of @nodejs//:yarn_node_repositories:

bazel run @nodejs//:yarn
bazel run @nodejs//:yarn --cwd foo
bazel run @nodejs//:yarn --cwd bar

…yarn/npm]_node_repositories Also, * create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets * create @nodejs//:node_files, @nodejs//:npm_files & @nodejs//:yarn_files filegroups * cleanup & simplify node_repositories.bzl code * cleanup & simplify @nodejs//:BUILD.bazel file BREAKING CHANGES: bazel run @nodejs//:npm is replaced with bazel run @nodejs//:npm_node_repositories and bazel run @nodejs//:yarn is replaced with bazel run @nodejs//:yarn_node_repositories. @nodejs//:yarn and @nodejs//:npm now run yarn & npm in the current working directory instead of on all of the package.json files in node_repositories(). @nodejs//:bin/node & @nodejs//:bin/node.cmd (on Windows) are no longer valid targets. Use @nodejs//:node instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/node. @nodejs//:bin/yarn & @nodejs//:bin/yarn.cmd (on Windows) are no longer valid targets. Use @nodejs//:yarn instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/yarn. @nodejs//:bin/npm & @nodejs//:bin/npm.cmd (on Windows) are no longer valid targets. Use @nodejs//:npm instead on all platforms. You can still call the old targets in their platform specific node repositories such as @nodejs_darwin_amd64//:bin/npm. [dc57136] feat(builtin): create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets

googlebot added the cla: yes label Nov 21, 2019

gregmagolan force-pushed the fixup_node_repositories branch from 87076e5 to d997b1c Compare November 21, 2019 07:44

gregmagolan changed the title ~~feat(builtin): create @nodejs//:npm_bin, @nodejs//:npx_bin & @nodejs//:yarn_bin targets~~ feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories Nov 21, 2019

gregmagolan force-pushed the fixup_node_repositories branch from d997b1c to 6f82338 Compare November 21, 2019 08:12

gregmagolan marked this pull request as ready for review November 21, 2019 19:42

gregmagolan requested review from alexeagle and soldair as code owners November 21, 2019 19:42

gregmagolan force-pushed the fixup_node_repositories branch from 6f82338 to 5d88741 Compare November 21, 2019 22:42

gregmagolan force-pushed the fixup_node_repositories branch from 5d88741 to cb93b59 Compare November 21, 2019 23:21

alexeagle approved these changes Nov 22, 2019

View reviewed changes

gregmagolan merged commit 01079a3 into bazel-contrib:master Nov 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories #1369

feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories #1369

gregmagolan commented Nov 21, 2019 •

edited

Loading

alexeagle commented Nov 21, 2019

gregmagolan commented Nov 21, 2019 •

edited

Loading

feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories #1369

feat(builtin): rename @nodejs//:npm and @nodejs//:yarn to @nodejs//:[yarn/npm]_node_repositories #1369

Conversation

gregmagolan commented Nov 21, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle commented Nov 21, 2019

gregmagolan commented Nov 21, 2019 • edited Loading

gregmagolan commented Nov 21, 2019 •

edited

Loading

gregmagolan commented Nov 21, 2019 •

edited

Loading